Skip to content

fix(wgpu-hal/vk) don't use sequential bindings if pipeline layout is set explicitly - #9353

Open
dylanblokhuis wants to merge 3 commits into
gfx-rs:trunkfrom
dylanblokhuis:vk-optional-sequential-bindings
Open

fix(wgpu-hal/vk) don't use sequential bindings if pipeline layout is set explicitly#9353
dylanblokhuis wants to merge 3 commits into
gfx-rs:trunkfrom
dylanblokhuis:vk-optional-sequential-bindings

Conversation

@dylanblokhuis

@dylanblokhuis dylanblokhuis commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Connections
#9326

Description
Currently descriptor bindings by default get built in a sequential fashion, not matching the binding indices that the user has set manually.

Testing
Tested some examples that use manual pipeline layouts.

Squash or Rebase?
Squash

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@dylanblokhuis dylanblokhuis changed the title feat(wgpu-hal/vk) don't use sequential bindings if pipeline layout is set explicitly fix(wgpu-hal/vk) don't use sequential bindings if pipeline layout is set explicitly Apr 1, 2026
@inner-daemons
inner-daemons self-requested a review April 2, 2026 05:07
@inner-daemons inner-daemons self-assigned this Apr 2, 2026
@dylanblokhuis
dylanblokhuis marked this pull request as ready for review April 2, 2026 13:02
@andyleiserson

Copy link
Copy Markdown
Contributor

Does it make sense to apply this treatment unconditionally if using passthrough shaders? Even if so, I guess we still need a flag when creating the layout, since it's not otherwise known at that point if the layout is going to be used with a passthrough shader. The current BindGroupLayoutFlags doesn't really work for that because it's only used between core and hal.

There should probably be some additional validation along the lines of checking that the binding layouts are consistent with the type of shader, and allowing the "use this interface exactly" mode only when the passthrough shader feature is available.

Unless I'm misunderstanding something, it seems better to have the flag activate the exceptional behavior (i.e. PRESERVE_BINDING_INDICES) rather than deactivate the exceptional behavior?

@inner-daemons

Copy link
Copy Markdown
Collaborator

Can you add a test demonstrating what is fixed? Also, can you say whether this only affects passthrough shaders?

@inner-daemons

Copy link
Copy Markdown
Collaborator

Also, apologies for the absolute silence from us (me).

@inner-daemons inner-daemons left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some questions in a comment above, another one here. Will wait for answers before I review the code since I don't yet fully understand what its trying to accomplish.

Comment thread wgpu-hal/src/lib.rs
@dylanblokhuis

Copy link
Copy Markdown
Contributor Author

Can you add a test demonstrating what is fixed? Also, can you say whether this only affects passthrough shaders?

The PR was incomplete, my bad, sorry.

So whenever the origin of a bind group layout is derived, e.g. from the user relying on Naga's reflection. It will use the sequential bindings in Vulkan, since the bindings were never set explicitly by the user, this shouldn't break anything.

So whenever a bind group layout has been set by defining the bindings manually, the vulkan backend will set the descriptors at the same indices as the user declared in their binding.

Why would you want this? When using passthrough shaders, you'd want the user that is supplying SPIRV to expect their bindings to match.

Currently I didn't find any easy way to propagate a flag when the user has used shader passthrough in wgpu, if this is possible, how could that look like?

@inner-daemons

Copy link
Copy Markdown
Collaborator

@dylanblokhuis At the pipeline layout point you don't yet know the exact shader. A single pipeline layout can be used by both a naga-compiled shader and a passthrough shader. I'm not sure exactly how to handle this.

On a semi-related note, I'm gonna be thinking through a precompiled shaders API, would you be ok getting pinged for discussion of that?

@andyleiserson

Copy link
Copy Markdown
Contributor

Unfortunately I don't think bgl::Origin::Derived works as the control for this. Passthrough shaders will imply explicit BGLs, but not the other way around. And we will want to fundamentally block web content from being able to specify low-level shader interface details, not just stash them behind a flag that it wouldn't normally set.

@dylanblokhuis

Copy link
Copy Markdown
Contributor Author

@dylanblokhuis At the pipeline layout point you don't yet know the exact shader. A single pipeline layout can be used by both a naga-compiled shader and a passthrough shader. I'm not sure exactly how to handle this.

On a semi-related note, I'm gonna be thinking through a precompiled shaders API, would you be ok getting pinged for discussion of that?

Yeah I guess the precompiled shaders API could probably solve this PR by itself, if there's some way for the user to supply their own mappings or heuristic based on backend.

@inner-daemons

Copy link
Copy Markdown
Collaborator

Precompiled shaders are a ways off, so if you need this lets not wait. But my vision for the API is a little bit complicated, which I think is necessary to have powerful passthrough shaders.

@andyleiserson andyleiserson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have new feedback, just marking as changes requested for triage purposes.

@inner-daemons
inner-daemons self-requested a review May 27, 2026 18:06
@inner-daemons

Copy link
Copy Markdown
Collaborator

I will get to this PR this week, sorry for the long delay.

@inner-daemons inner-daemons left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a changelog entry, otherwise looks good to me, though Andy's comment still stands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants